Skip to content

Conversation

@amin1377
Copy link
Contributor

This PR depends on PR #3342. Please wait until that PR is merged.

In this PR, node_tilable_track_nums_ is moved into the RR graph storage instead of residing in the RR graph builder, since this data structure (when using a tileable RR graph) needs to remain valid for the entire lifetime of the RR graph.

Additionally, a method is added to facilitate removing nodes from the RR graph.

@github-actions github-actions bot added VPR VPR FPGA Placement & Routing Tool docs Documentation lang-cpp C/C++ code labels Nov 17, 2025
@AmirhosseinPoolad
Copy link
Contributor

What's the use case of removing nodes?

@amin1377
Copy link
Contributor Author

What's the use case of removing nodes?

For the custom RR graph, in the other PR there are channel nodes, particularly around the perimeter of the device, that don’t have any fan-in. We need to remove these nodes after the graph is created. I considered not adding them in the first place, but that’s difficult to determine before the edges are added.

As we discussed earlier, I’m trying to break the custom RR graph PR into multiple smaller PRs to make the review process easier.

@amin1377 amin1377 changed the title [WIP] Move node_tilable_track_nums_ to rr_graph_storage and add remove_node Move node_tilable_track_nums_ to rr_graph_storage and add remove_node Nov 20, 2025
@github-actions github-actions bot added the libarchfpga Library for handling FPGA Architecture descriptions label Nov 20, 2025
Copy link
Contributor

@AmirhosseinPoolad AmirhosseinPoolad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR Amin. Had some comments.

Copy link
Contributor

@AmirhosseinPoolad AmirhosseinPoolad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Misclicked, didn't mean to approve. Sorry!

@amin1377
Copy link
Contributor Author

Misclicked, didn't mean to approve. Sorry!

I’ll happily take that approval anyway 😁

@amin1377
Copy link
Contributor Author

@AmirhosseinPoolad: I’ve addressed your comments. I’d appreciate it if you could take another look at the PR and let me know if you have any further feedback. It would be great if we could finalize this PR today!

Copy link
Contributor

@AmirhosseinPoolad AmirhosseinPoolad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for being late! This looks good other than the lambda capture list (super dangerous imo to capture everything by non-const reference and it wouldn't be clear to future maintainers which outside variables the lambda is actually using without going through the lambda code). I still need another person to look at the flag change.

@amin1377
Copy link
Contributor Author

Thanks, @AmirhosseinPoolad . I’ve applied the changes you recommended. Please let me know if you have any further comments.

Copy link
Contributor

@AmirhosseinPoolad AmirhosseinPoolad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

@amin1377 amin1377 merged commit 08f357f into master Nov 28, 2025
30 checks passed
@amin1377 amin1377 deleted the remove_node branch November 28, 2025 21:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

docs Documentation lang-cpp C/C++ code libarchfpga Library for handling FPGA Architecture descriptions VPR VPR FPGA Placement & Routing Tool

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants